-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate all tests code to TypeScript #2609
Conversation
@dotansimha Looks gorgeous, thanks a lot 👍
Can you please make it a separate PR? |
@dotansimha You can turn off specific rules just for tests see bottom of |
@dotansimha We don't use But AFAIK |
@IvanGoncharov Working on a rebase now, I saw you changed some of the eslint/tslint rules so I'm adjusting to it. |
@IvanGoncharov rebased and pushed fixes for all notes :) |
@dotansimha Also, don't worry about rebasing I will temporarily stop committing or merging prs for the next couple of days. |
@dotansimha I understand. Also, can you please go through your changes split other stuff not directly related to TS conversion e.g. It will save a lot of time on back and forth comments between us and will make my life so much easier because I need to review every single line and my head is exploding 🤯 |
@dotansimha Need to merge some PRs but will try to not touch tests. |
@IvanGoncharov moved all I think you can merge other PRs if you need to, I'll need a rebase anyway after merging the #2616. Regarding the |
@IvanGoncharov done, but I'm not sure why codecov still complains about missing coverage? I can't see there anything missing. Am I missing something? |
@dotansimha Investigating this at the moment, think I know the solution would fix it on branch. |
extracted from graphql#2609
extracted from graphql#2609
@IvanGoncharov done :) So I think the next steps is to merge #2616 and #2624 , then rebase this one and make sure coverage works, right? |
@dotansimha Changes are too big and I can constantly discover new issues in this PR. First, such PR should disable all falling eslint rules and disable all failing checks in Meanwhile, I will work on improving the situation with coverage and also removing |
I see what you mean @IvanGoncharov , thank you. Please note that most files has been transformed to TS in terms of syntax, but some needed additional minor changes to support TS. We covered the rest of the changes and we understood why they are needed (additional tsconfig, changes to eslint, changes to cspell and a fix for codecov - all the rest are just transformation from Flow to TS). Personally, I don't really see the benefit of 10 separate PRs over 1 working PR that already exists, especially if they eventually do the same and will go into the same target branch (instead of It just feels like we'll always be in a state of "moving to TS" instead of just do it... What do you think I should start with? what should the first PR contain and how we should gradually move there? |
@IvanGoncharov I created this: #2634 , starting to gradually make it compatible. |
@dotansimha I'm ok to merge syntax changes together with disabling ESLint rules as described in my previous comment:
Please disable rules only if they are failing on syntax changes.
I reviewed only 20% of changes PR that contains only syntax changes can be reviewed in less than an hour. |
This can be closed considering that the code base has been converted to TS. Thank you @dotansimha for your work on this, which has been integrated into other PRs, as well as for your energy and initiative in pushing forward the migration. |
This PR is based on the TypeScript migration path from #2104
This PR is also inspired by #2139 , but it uses the most recent codebase, and migrates all tests files to TS.
The changes in this PR includes the following:
TS Migration
.d.ts
files for test utils and other.js
files that are in use by both Flow code and TypeScript tests.tsconfig.json
, this is needed because we are going to compile TypeScript files, and thetsconfig.json
that located undersrc
is different, and used bydtslint
only.ts-node
and usets-node/register
in Mocha configuration to support.ts
files.